Skip to content

AppVeyor: only load minimal set of exts; don't run unsupported test s… #7150

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

AppVeyor: only load minimal set of exts; don't run unsupported test s… #7150

wants to merge 7 commits into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Jun 14, 2021

…uites

@cmb69 cmb69 mentioned this pull request Jun 14, 2021
Closed
nikic
nikic reviewed Jun 14, 2021
View reviewed changes
appveyor/test_task.bat Outdated

rem overwrite tmp-php.ini
echo extension_dir=%PHP_BUILD_DIR% > %PHP_BUILD_DIR%\tmp-php.ini
echo opcache.file_cache=%PHP_BUILD_DIR%\test_file_cache >> %PHP_BUILD_DIR%\tmp-php.ini
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that part of the old tmp-php.ini? So our opcache test on Windows is actually testing file cache and not SHM cache?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opcache.file_cache does not disable SHM caching on Windows; that would require opcache.file_cache_only to be enabled (but defaults to off). The file cache is relevant for opcache.file_cache_fallback which is enabled by default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that makes sense.

@nikic
Copy link
Member

nikic commented Jun 14, 2021

Part of the test failures are due to duplicate exts in EXTENSIONS (should possibly check that and BORK). Most of them are because we don't pass through loaded extensions to proc_open based server tests...

@cmb69
Copy link
Member Author

cmb69 commented Jun 14, 2021

Ugh! I'll have a look at this, but it may take some days.

@nikic
Copy link
Member

nikic commented Jun 14, 2021

I think I've fixed all the incorrect/missing EXTENSIONS sections, so if you rebase this should have just the proc_open issues now. Not entirely sure what to do about those. We should really forward -c/-d from the run-tests invocation of the main test file (for this and other reasons), but I'm not sure how exactly that should work...

@nikic
Copy link
Member

nikic commented Jun 14, 2021

Though to make some quick progress here, it may make sense to keep an unconditional load of openssl in particular for now. Seems to be the only one affected from a quick glance.

@cmb69
Copy link
Member Author

cmb69 commented Jun 14, 2021

I did that now. Let's wait for the test results.

@nikic
Copy link
Member

nikic commented Jun 14, 2021

Down to two failures:

Bug #77578 (Crash when php unload) [C:\projects\php-src\ext\com_dotnet\tests\bug77578.phpt]
Bug #68547 (Exif Header component value check error) [C:\projects\php-src\ext\exif\tests\bug68547.phpt]

The first one is an exec() test so has the forwarding problem.

I'm a bit stumped on the second one though. Not seeing anything that should be affected by extension loading there. It failed on both jobs though, so it doesn't look spurious either.

@cmb69
Copy link
Member Author

cmb69 commented Jun 14, 2021

I'm a bit stumped on the second one though. Not seeing anything that should be affected by extension loading there. It failed on both jobs though, so it doesn't look spurious either.

I guess we need to load mbstring as well.

@nikic
Copy link
Member

nikic commented Jun 14, 2021

@cmb69 Oh, nice catch! I wasn't aware exif has an optional dependency on mbstring.

Looks like only the com_dotnet test fails now, so I'd suggest to also unconditionally load that ext for now, and then this should be good to go.

@cmb69
Copy link
Member Author

cmb69 commented Jun 14, 2021

I've fixed the COM test by loading com_dotnet via command line argument. I've just pushed a final commit to get rid of the auto-generated php.ini altogether.

It seems to me that the test suite runs considerably faster now. :)

nikic
nikic approved these changes Jun 14, 2021
View reviewed changes
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM assuming CI passes. Thanks!

@cmb69
Copy link
Member Author

cmb69 commented Jun 14, 2021

assuming CI passes.

;) I'll need to have a closer look; also, there are more tests skipped now.

@nikic
Copy link
Member

nikic commented Jun 14, 2021

Presumably we're missing this part:

php-src/win32/build/confutils.js

Lines 2039 to 2044 in 9871a62

FSO.CreateFolder(dir);
/* Fallback is implied, if filecache is enabled. */
INI.WriteLine("opcache.file_cache=" + dir);
INI.WriteLine("opcache.enable=1");
INI.WriteLine("opcache.enable_cli=1");

@cmb69
Copy link
Member Author

cmb69 commented Jun 14, 2021

Ah, right!

@cmb69
Copy link
Member Author

cmb69 commented Jun 14, 2021

CI is green, and results look okay, so I think this is good to be merged.

@cmb69 cmb69 marked this pull request as ready for review June 14, 2021 21:26
@cmb69 cmb69 closed this in 139a73b Jun 14, 2021
@cmb69 cmb69 deleted the cmb/appveyor branch June 14, 2021 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikic nikic nikic approved these changes

Assignees
No one assigned
Labels
Category: Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cmb69 @nikic @krakjoe